Codecov Report

All modified and coverable lines are covered by tests ✅

How can we test that this is not destroying the performances of UPGrad? Do you think unit tests are sufficient for this? I think we should also use the trajectories project to verify empirically that the trajectories still look good (that will depend on the value of norm_eps of course). Maybe we should even run one of the experiments of jde to verify that we can still obtain roughly similar performance.

I think we would also have to change the default value of norm_eps in UPGrad and DualProj.

Lastly, this clearly deserves a changelog entry. Arguably, this is not a breaking change so we can remove this label if you think it's not so different.

As a partial answer to your question, I added a test that proves that project_weights is scaling invariant on the Gramian. So the normalization of the Gramian does not change anything in the normal regime (not too high, not too low).

Additionally, the scaling factor that we had versus what we have have a ratio of at most $\sqrt{m}$. This is because we use to scale by the largest singular value which is the square root of the maximal eigen value of the Gramian. Now we scale by the square root of the trace of the Gramian. The trace of the gramian is the sum of the eigen values and we have $\sigma_{\max}(G)\leq \text{Tr}(G)\leq m \sigma_{\max} (G)$. Taking the square root we get that our new normalization is between the largest singular value and $\sqrt{m}$ times this same singular value.

Additionally, the scaling factor that we had versus what we have have a ratio of at most m . This is because we use to scale by the largest singular value which is the square root of the maximal eigen value of the Gramian. Now we scale by the square root of the trace of the Gramian. The trace of the gramian is the sum of the eigen values and we have σ max ( G ) ≤ Tr ( G ) ≤ m σ max ( G ) . Taking the square root we get that our new normalization is between the largest singular value and m times this same singular value.

Very good to know that thanks! So assuming m=100 (which is kind of the maximum realistic value for m that users would have), the difference would be at max of a factor 10. I think this is fine, because from my past experience the norm_eps was not so sensitive and changing it by a factor 10 should not affect the results that much.

Apart from my comments and todos, LGTM.

For me, we could have another PR looking at norm_eps and reg_eps, I do not know (yet) how to assess the correct values for those, and I need to think about it a lot more, the current values are fine for now, we will do JDE and trajectories at that moment, this is out of scope. For the atol of the LUS, this will be related to a refactor of the property testers and is also out of scope for this PR.
So for me, this is now ready to merge.

Waiting for Windows tests, and then we can merge!